WIP: Feature/xpress solver for mathopt #137
Conversation
| const absl::Span<const double> ub, | ||
| const absl::Span<const char> vtype) { | ||
| CHECK_EQ(vind.size(), vval.size()); | ||
| const int num_vars = static_cast<int>(lb.size()); |
There was a problem hiding this comment.
we are not quite ready yet, but we are working up to int64 support (models with more than 2**31 variables). Not sure if xpress supports this, but if you do, maybe use int64_t instead of int here.
There was a problem hiding this comment.
XPRESS does support this but through separate API calls of course (in this instance, XPRSaddcols64 instead of XPRSaddcols). If it's OK for you, I'll add a TODO here to look into it, and add support for int64 later (it will need some reflexion on how to support both 32 and 64 archs, maybe we'll wait for you to do it for gurobi and then copy)
There was a problem hiding this comment.
Xpress supports only 64bit counts of non-zeros. Number of variables, rows, etc. are all limited to 32bit signed integers. Since you are not adding any non-zeros here, the code is already safe for the kind of 64bit stuff that Xpress supports.
I still suggest that instead of static_cast<int>(lb.size()) you use a function that checks for overflow (lb.size() larger than INT_MAX) and raises an error if a user tries to create that many variables. I guess such a function will come in handy in other places as well.
There was a problem hiding this comment.
Thanks a million for doing this!
I made a first review pass over the implementation (not the tests). In my comments I assume that we want to code against a somewhat recent version of Xpress (that already has XPRSoptmize(), XPRSgetsolution(), XPRSgetduals(), ...).
| return absl::OkStatus(); | ||
| } | ||
| char errmsg[512]; | ||
| XPRSgetlasterror(xpress_model_, errmsg); |
There was a problem hiding this comment.
In theory XPRSgetlasterror() can fail. To make this rock solid I suggest to check the return value of that function and set errmsg to some generic string in case the function returns non-zero.
| bool correctlyLoaded = initXpressEnv(); | ||
| CHECK(correctlyLoaded); | ||
| XPRSprob* model; | ||
| CHECK_EQ(kXpressOk, XPRScreateprob(model)); |
There was a problem hiding this comment.
This looks wrong to me. XPRSprob already is a pointer type. I think this should read
XPRSprob model;
CHECK_EQ(kXpressOk, XPRScreateprob(&model));
DCHECK(model != nullptr);
return absl::WrapUnique(new Xpress(model));
| const absl::Span<const double> ub, | ||
| const absl::Span<const char> vtype) { | ||
| CHECK_EQ(vind.size(), vval.size()); | ||
| const int num_vars = static_cast<int>(lb.size()); |
There was a problem hiding this comment.
Xpress supports only 64bit counts of non-zeros. Number of variables, rows, etc. are all limited to 32bit signed integers. Since you are not adding any non-zeros here, the code is already safe for the kind of 64bit stuff that Xpress supports.
I still suggest that instead of static_cast<int>(lb.size()) you use a function that checks for overflow (lb.size() larger than INT_MAX) and raises an error if a user tries to create that many variables. I guess such a function will come in handy in other places as well.
|
|
||
| const int n_cols = static_cast<int>(colind.size()); | ||
| auto c_colind = const_cast<int*>(colind.data()); | ||
| auto c_values = const_cast<double*>(values.data()); |
There was a problem hiding this comment.
Why const_cast? There is no need to cast away the const since XPRSchgobj() takes const arguments.
| const int n_coefs = static_cast<int>(rowind.size()); | ||
| auto c_rowind = const_cast<int*>(rowind.data()); | ||
| auto c_colind = const_cast<int*>(colind.data()); | ||
| auto c_values = const_cast<double*>(values.data()); |
There was a problem hiding this comment.
Again, no need to cast away const. You should be able to directly pass rowind.data() etc. to XPRSchgmcoef().
Here I suggest to use XPRSchgmcoef64(). All data is already in the right format. You only have to change n_coefs to a 64bit integer. This then supports 64bit non-zero counts.
| return xpress_->AddConstrs(constraint_sense, constraint_rhs, constraint_rng); | ||
| } | ||
|
|
||
| absl::Status XpressSolver::AddSingleObjective(const ObjectiveProto& objective) { |
There was a problem hiding this comment.
What is this function supposed to do? At the moment (unless I am mistaken) it only changes the coefficients of variables that appear in objective. If a variable is not in objective then its objective coefficient won't be changed. In particular, it would not be zeroed. Or is objective always dense?
| } | ||
| // Post-solve | ||
| if (!(is_mip_ ? (xpress_status_ == XPRS_MIP_OPTIMAL) | ||
| : (xpress_status_ == XPRS_LP_OPTIMAL))) { |
There was a problem hiding this comment.
As I commented in the other file, the preferred way to solve is XPRSoptimize() and then just check the solve and solution status returned by this function.
| int lp_iter_limit = parameters.has_iteration_limit() | ||
| ? parameters.iteration_limit() | ||
| : 2147483647; | ||
| // XPRESS default value for XPRS_BARITERLIMIT is 500 |
There was a problem hiding this comment.
In order to get the default values, you should rather query the attributes right after calling XPRScreateprob(). This seems more future proof.
| } | ||
|
|
||
| absl::StatusOr<double> XpressSolver::GetBestDualBound() const { | ||
| // TODO: setting LP primal value as best dual bound. Can this be improved? |
|
|
||
| bool XpressSolver::isFeasible() const { | ||
| if (is_mip_) { | ||
| return xpress_status_ == XPRS_MIP_OPTIMAL; |
There was a problem hiding this comment.
There is another status that indicates "feasible but not proven optimal": XPRS_MIP_SOLUTION, see here.
Hi Daniel! Thanks a lot for you valuable review! Best regards ! |
No description provided.